Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added pickup and drop actions #38

Merged

Conversation

landrumb
Copy link
Contributor

pickup and drop functions were added in response to issue #36, an inv attribute was added to Agent, a new abstract class Item <: AbstractObject was added to indicate objects that could be picked up, and Key and Gem were changed to inherit from it.

Outside of objects.jl, doorkey.jl was changed to use the pickup function and inv attribute of Agent, but its functionality is the same as before.

As is, when an agent picks up or drops their inventory is up to the environment to decide. In doorkey.jl I left the existing behavior where walking over the key picks it up, but in an environment such as ObstructedMaze (#34) where items have to be dropped, there needs to be an interface besides walking over the object one wants to pick up. If the default Null object was replaced with Empty <: Item, such that empty spaces could be picked up, it would allow picking up and dropping items by replacing the Empty object in the inventory with something else and vice versa, but this would not work with the automatic pickup used in doorkey.jl, because the key would be automatically dropped on every forward movement.

@codecov-io
Copy link

codecov-io commented Oct 10, 2020

Codecov Report

Merging #38 into master will decrease coverage by 3.22%.
The diff coverage is 7.31%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #38      +/-   ##
==========================================
- Coverage   36.06%   32.83%   -3.23%     
==========================================
  Files          13       13              
  Lines         305      335      +30     
==========================================
  Hits          110      110              
- Misses        195      225      +30     
Impacted Files Coverage Δ
src/envs/doorkey.jl 0.00% <0.00%> (ø)
src/objects.jl 8.47% <8.57%> (-8.77%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 495419c...0fc0d26. Read the comment docs.

src/objects.jl Outdated Show resolved Hide resolved
src/objects.jl Outdated
const GEM = Gem()
Base.convert(::Type{Char}, ::Gem) = '♦'
get_color(::Gem) = :magenta

Base.@kwdef mutable struct Agent <: AbstractObject
color::Symbol=:red
dir::LRUD
inv::Item=NULL
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe create a new type of agent as you suggest in the related issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In implementing this, should I create an AbstractAgent <: AbstractObject to simplify annotations for pickup functions and make additional agent types easier to add?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that'd be better.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implemented in my last commit. I made some design decisions that I'm not confident were ideal, such as the way I made the constructor for Array_agent, and my choice of Array_agent as the name, but it works.

src/objects.jl Outdated Show resolved Hide resolved
@landrumb landrumb marked this pull request as draft October 11, 2020 18:57
src/objects.jl Outdated Show resolved Hide resolved
@landrumb
Copy link
Contributor Author

I am not confident that my implementation of traits is correct. I would appreciate if someone could look over my last commit and confirm that it's done correctly.

@landrumb landrumb marked this pull request as ready for review October 12, 2020 13:00
src/objects.jl Outdated Show resolved Hide resolved
src/objects.jl Outdated Show resolved Hide resolved
src/objects.jl Outdated Show resolved Hide resolved
src/objects.jl Outdated Show resolved Hide resolved
src/objects.jl Outdated Show resolved Hide resolved
Copy link
Member

@findmyway findmyway left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think most usages are correct! Well done!

My only concern is the name of Item, though I also can not come up with a better name yet...

@sriyash421
Copy link
Collaborator

@findmyway @landrumb Sorry, I might be repeating something that might have been discussed earlier but I wanted to ask something. Why can't we use abstract objects to define traits such as Pickable = Union{Key, Gem} and check if Obj <: Pickable ??

@landrumb
Copy link
Contributor Author

@sriyash421 If I understand your question, what you're describing is essentially the same as what we do with traits, but the advantage of using traits is that you can more fluidly build it into the dispatch for functions that rely on the type annotation. However, I could be wrong as I've only known about traits for about 2 days. @findmyway can confirm or deny if that's the reason we're using traits, as he's more knowledgeable on the topic.

@findmyway
Copy link
Member

Yes @landrumb is mostly right here.
@sriyash421 Suppose one wants to create a new environment and needs to create a new object. And this object happens to be a pickable object. If we adopt the union method, then one has no way but to change the source code of our package. But with the trait way, one can simply extend the isitem method.

@landrumb landrumb requested a review from findmyway October 13, 2020 11:25
src/objects.jl Outdated Show resolved Hide resolved
@findmyway findmyway merged commit 910a0cf into JuliaReinforcementLearning:master Oct 13, 2020
@findmyway findmyway linked an issue Oct 13, 2020 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adding pickup and drop action to the agent
4 participants